Merge Request

Alexandre me dit : « Le contenu de Speed of Code Reviews (https://google.github.io/eng-practices/review/reviewer/speed.html) ressemble à ce dont tu faisais la promotion dans notre précédente équipe ».

En effet, après lecture, les recommandations de cette documentation font partie de ma doctrine d'artisan développeur.

Note: j'ai remplacé CL qui signifie Changelist par Merge Request.

When code reviews are slow, several things happen:

• The velocity of the team as a whole is decreased. Yes, the individual who doesn’t respond quickly to the review gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each Merge Request waits for review and re-review.
• Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the Merge Request each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health), but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.
• Code health can be impacted. When reviews are slow, there is increased pressure to allow developers to submit Merge Request that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing Merge Request.

source

J'ai fait le même constat et je trouve que cette section explique très bien les conséquences 👍️.

How Fast Should Code Reviews Be?

If you are not in the middle of a focused task, you should do a code review shortly after it comes in.

One business day is the maximum time it should take to respond to a code review request (i.e., first thing the next morning).

Following these guidelines means that a typical Merge Request should get multiple rounds of review (if needed) within a single day.

source

Je partage et recommande cette pratique 👍️.

If you are too busy to do a full review on a Merge Request when it comes in, you can still send a quick response that lets the developer know when you will get to it, suggest other reviewers who might be able to respond more quickly.

source

👍️

Large Merge Request

If somebody sends you a code review that is so large you’re not sure when you will be able to have time to review it, your typical response should be to ask the developer to split the Merge Request into several smaller Merge Requests that build on each other, instead of one huge Merge Request that has to be reviewed all at once. This is usually possible and very helpful to reviewers, even if it takes additional work from the developer.

source

Je partage très fortement cette recommandation et je pense que c'est celle que j'avais le plus de difficulté à faire accepter par les nouveaux développeurs.

Quand je code, j'essaie de garder à l'esprit que mon objectif est de faciliter au maximum le travail du reviewer plutôt que de chercher à minimiser mes propres efforts.

J'ai sans doute acquis cet état d'esprit du monde open source. En effet, l'un des principaux défis lors d'une contribution à un projet open source est de faire accepter son patch par le mainteneur. On comprend rapidement qu'un patch doit être simple à comprendre et rapide à intégrer pour maximiser ses chances d'acceptation.

Un bon patch doit remplir un objectif unique et ne contenir que les modifications strictement nécessaires pour l'atteindre.

Je suis convaincu que si une équipe de développeurs applique ces principes issus de l'open source dans leur contexte professionnel, leur efficacité collective s'en trouvera grandement améliorée.

Par ailleurs, une Merge Request de taille réduite présente plusieurs avantages concrets :

• elle est non seulement plus simple à rebase,
• mais elle a aussi plus de chances d'être mergée rapidement.

Cela permet à l'équipe de bénéficier plus rapidement des améliorations apportées, qu'il s'agisse de corrections de bugs ou de nouvelles fonctionnalités.

Lorsque vous collaborez avec moi, je préfère recevoir les extraits de vos textes Markdown, vos codes sources, et le contenu de votre terminal au format texte plutôt que sous forme de captures d'écran.

Voici ci-dessous six inconvénients liés à l'utilisation de captures d'écran.

1. Impossibilité de copier le contenu

Lorsque vous partagez des lignes de commande, des configurations ou des messages d'erreur en capture d'écran, je ne peux pas copier le contenu pour le tester ou effectuer des recherches. Cela m'oblige à ressaisir manuellement le texte, avec le risque d'introduire des erreurs.

De plus, il m'est impossible de citer précisément une partie de votre contenu lors de nos échanges.

2. Impossible de cliquer sur les liens

3. Recherche inefficace

Les outils comme GitHub, GitLab, Slack, Zulip, Mattermost, Signal, WhatsApp, ainsi que les moteurs de recherche dans les pages des navigateurs, ne permettent pas de rechercher du texte présent dans des images. Le format texte, en revanche, est entièrement indexable et facilitera la recherche d'informations.

4. Consommation de mémoire multipliée par 100

Les captures écrans consomment beaucoup plus d'espace disque que du texte.

Prenons l'exemple d'un extrait de terminal au format texte :

# docker info
Client: Docker Engine - Community
 Version:    27.1.2
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  v0.16.2
    Path:     /usr/libexec/docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  v2.29.1
    Path:     /usr/libexec/docker/cli-plugins/docker-compose

Server:
 Containers: 194
  Running: 194
  Paused: 0
  Stopped: 0
 Images: 34
 Server Version: 27.1.2
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Using metacopy: false
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2

Cet extrait consomme 682 octets de mémoire.

Je viens de vérifier : une capture d'écran équivalente consomme 83 000 octets de mémoire, soit 121 fois plus d'espace disque qu'un simple extrait de texte. Pour ceux qui utilisent des écrans haute résolution, comme les écrans Retina, cette consommation peut même être multipliée par 4.

Il est regrettable de saturer 100 fois plus rapidement l'espace disque sur des services comme GitHub, GitLab, Slack, Zulip, Mattermost, etc., pour une expérience utilisateur finalement moins optimale.

5. Chargement moins rapide des pages

Ceci est surtout impactant pour les issues ou Merge Request avec beaucoup de commentaires.

6. Occupation excessive de l'espace écran

Les captures d'écran, en particulier celles prises sur des écrans Retina, occupent souvent un espace d'écran excessif dans la fenêtre du navigateur, rendant la lecture des commentaires plus difficile et nuisant à la fluidité de la navigation.

Depuis des années, j'essaie de suivre avec rigueur la doctrine suivante dans les projets utilisant le workflow Trunk-Based Development.

• La branche trunk doit toujours être stable et contenir uniquement du code fonctionnel.
• Le code obsolète ou inutilisé doit être supprimé de la branche trunk.
• Aucun code commenté ne doit figurer dans la branche trunk.
• La branche trunk ne contient pas de tests qui échouent.

Pourquoi ?

• Pour éviter qu'un développeur perde du temps à essayer de faire fonctionner quelque chose qui n'est pas en état de marche.
• Pour éviter qu'un développeur refactore du code mort — j'ai observé à nouveau cela, il n'y a pas longtemps 😔. Quand le développeur fini par le découvrir, il est généralement très frustré.
• Pour éviter l'installation et la mise à jour de bibliothèques qui alourdissent inutilement le projet.
• Pour prévenir une perte de confiance dans le projet (voir l'hypothèse de la vitre brisée).

Et si j'ai besoin de ce code plus tard ?

Tout d'abord, je vous réponds "YAGNI" 🙂.

Plus sérieusement, ma réponse est que votre code ne sera pas perdu étant donné qu'il est versionné dans votre repository.

Si le code commenté est en cours de développement, alors je suggère d'extraire ce code en préparation dans une Merge Request et de la merger quand elle sera prête.

Trouvez le bon équilibre

Un morceau de code commenté ou un test qui échouent peut tout à fait rester dans trunk sur une courte période. Dans ce cas, je conseille d'ajouter en commentaire un lien vers l'issue de dette technique qui détaille l'action prévue.

Après avoir rédigé la note Commit Cavalier, #JaiDécouvert le concept de des projets de loi omnibus.

« Depuis les années 1980, cependant, les projets de loi omnibus sont devenus plus courants : ces projets de loi contiennent des dispositions, parfois importantes, sur un éventail de domaines politiques. »

-- from

Ceci m'a fait penser aux Merge Requests qui contiennent de nombreux petits commits, souvent de refactoring, qu'il serait fastidieux de passer en revue individuellement.

Je pense que je vais nommer ces Merge Request, des Merge Request Omnibus, ce néologisme sera une de mes marques idiosyncrasiques 😉.

J'ai donc décidé de baptiser ces Merge Requests des Merge Requests Omnibus. Ce néologisme deviendra l'une de mes marques idiosyncrasiques 😉.

Je tente ici de présenter la notion de Git Commit dit "cavalier" en la reliant au concept de Cavalier Législatif.

Un cavalier législatif est un article de loi qui introduit des dispositions qui n'ont rien à voir avec le sujet traité par le projet de loi.

Ces articles sont souvent utilisés afin de faire passer des dispositions législatives sans éveiller l'attention de ceux qui pourraient s'y opposer.

-- from

Dans le contexte de développement logiciel, un Commit Cavalier désigne un commit inséré dans une Pull Request ou Merge Request qui n’a aucun lien direct avec l’objectif principal de celle-ci.

Cette pratique pose les problèmes suivants :

• Cela rend la Merge Request plus difficile à review ;
• Cela rend la Merge Request plus longue à review ;
• Cela lance des discussions sans lien avec l'objectif de la Merge Request ;
• Le Commit Cavalier devient "invisible" au reste de l'équipe ;
• L'auteur peut mettre la pression au reviewer pour merger son Commit Cavalier sous prétexe que la Merge Request doit être mergé rapidement.

Il va sans dire que cette pratique a le don de m'irriter profondément. Par respect pour mon reviewer et mon équipe, je veille scrupuleusement à ne jamais soumettre de commit cavalier.

Depuis 2012, je pratique exclusivement le Git Rebase Workflow pour tous mes projets de développement.

Concrètement :

• J'utilise git pull --rebase quand je travaille dans une branche, généralement une Pull Request ou Merge Request ;
• Je pousse régulièrement des commits en "work in progress" au fil de l'avancée de mon travail dans ma branche de développement avec la commande git commit -m "WIP"; git push ;
• Une fois le travail terminé, je squash mes commits à l'aide de git rebase -i HEAD~[NUMBER OF COMMITS] ;
• Ensuite, je rédige un commit message qui contient la description du changement et le numéro de l'issue ou de la merge request git commit --amend ;
• Enfin, j'effectue un Merge en Fast-Forward en utilisant l'interface de GitHub ou GitLab.

Pour cela, je paramètre GitLab de la façon suivante (navigation "Settings" => "General") :

Ou alors je paramètre GitHub de la façon suivante (navigation "Settings" => "General")

Les avantages de cette pratique

L'approche Rebase + Squash + Merge Fast-Forard permet de maintenir l'historique de changements linéaire, rendant celui-ci plus facile à lire et à comprendre.

L'historique ne contient aucun commit de fusion inutile.

Cela facilite la mise en place d'Intégration Continue.

Tous les problèmes, bugs, et conflits sont traités dans les branches, dans les Merge Request et jamais dans la branche main qui se doit d'être toujours stable, ce qui améliore grandement le travail en équipe.

Ce workflow est particulièrement puissant lorsque l'historique linéaire ne contient que des commit dit "atomic", c’est-à-dire : 1 issue = 1 merge request = 1 commit. Un commit est considéré comme "atomic" lorsqu'il ne contient qu'un seul type de changement cohérent, tel qu'une correction de bug, un refactoring ou l'implémentation d'une seule fonctionnalité.

À de rares exceptions près, le code source de la branche main doit rester stable et cohérent tout au long de l'historique des commits.

Cette discipline favorise un travail collaboratif de qualité, rendant plus compréhensible l'évolution du projet.

De plus, l'atomicité des commits facilite la revue des Merge Request et permet d'éviter les Commits Cavaliers.

Généralement je couple ce Git workflow au workflow nommé Trunk-Based Development.